Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding logical_to_disjunctive, moving GDP transformations from logical-to-linear to logical-to-disjunctive #2627

Merged
merged 57 commits into from
Dec 6, 2022

Conversation

emma58
Copy link
Contributor

@emma58 emma58 commented Nov 25, 2022

Fixes # .

Summary/Motivation:

The core.logical_to_linear transformation is very slow because it uses sympy to convert logical expressions to CNF. Since we don't really need them in CNF in order to write a MIP (which is the goal of the transformation), this PR adds a transformation to contrib.cp called logical_to_disjunctive that transforms logical constraints into disjunctive programs in the sense that 1) logical constraints on only Boolean variables are converted to algebraic constraints on binary variables, but 2) logical constraints on mixtures of Boolean and integer variables (such as exactly, atmost, and atleast) are transformed into disjunctions over binaries and integers. (Note to future us: We're using the term "disjunctive program" rather than GDP intentionally here, as the promise of the transformation is to no longer have any logical expressions in the model: only algebraic constraints and disjunctions of algebraic constraints.)

This reduces the gdp.bigm transformation time for a certain model that builds in 15 seconds from 129 seconds (current main branch) to 75 seconds (this branch).

NOTE: This is currently failing tests because of #2626 (because there are logical constraints in the pickle tests and logical_to_disjunctive fixes some variables.)

Changes proposed in this PR:

  • Adds contrib.logical_to_disjunctive transformation to contrib.cp
  • Deprecates core.logical_to_linear, mainly because it will not be easily extensible as we add to the set of supported nodes in the logical expression system. For example, exactly, atmost, and atleast are handled with ad hoc bigm-ing right now--we would have to keep adding special handling as we include alldiff, etc.
  • Transitions all the GDP transformations to use contrib.logical_to_disjunctive
  • Removes support for transforming BooleanVars declared on Disjuncts that are not used in expressions in the active tree being transformed.

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

emma58 and others added 28 commits November 11, 2022 13:58
…if it works yet because tests need rewriting too
… assertExpressionsStructurallyEqual (so that the same number as an int and as a float is equal)
…DP transformations because logical_to_disjunctive doesn't create them.
…isjunctive, but making an option for another transformation if desired
jsiirola
jsiirola previously approved these changes Nov 28, 2022
Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! One minor performance-related comment, but doesn't need to hold up this PR

@jsiirola jsiirola dismissed their stale review November 28, 2022 04:44

(missed that there were significant failing tests in Jenkins)

@emma58 emma58 changed the title Deprecating logical_to_linear, adding logical_to_disjunctive [WIP] Deprecating logical_to_linear, adding logical_to_disjunctive Nov 28, 2022
@emma58
Copy link
Contributor Author

emma58 commented Nov 28, 2022

Marking this as WIP for now--I didn't realize there are tests in core that call bigm and hull, so I need to fix those.

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great. One question: I do agree that we should switch GDP from logical_to_linear to using logical_to_disjunctive; however, do we want/need to deprecate logical_to_linear? While the implementation has several limitations, it does provide a somewhat different capability (the CNF form).

@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Base: 87.01% // Head: 87.05% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (5e885ca) compared to base (34f07c4).
Patch coverage: 95.69% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2627      +/-   ##
==========================================
+ Coverage   87.01%   87.05%   +0.04%     
==========================================
  Files         754      757       +3     
  Lines       84078    84360     +282     
==========================================
+ Hits        73160    73441     +281     
- Misses      10918    10919       +1     
Flag Coverage Δ
linux 84.48% <95.69%> (+0.05%) ⬆️
osx 74.91% <94.98%> (+0.08%) ⬆️
other 84.67% <95.69%> (+0.05%) ⬆️
win 81.84% <95.69%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyomo/contrib/gdpopt/branch_and_bound.py 76.59% <ø> (ø)
pyomo/environ/__init__.py 82.35% <ø> (ø)
pyomo/gdp/plugins/multiple_bigm.py 94.96% <75.00%> (-0.63%) ⬇️
pyomo/gdp/plugins/fix_disjuncts.py 97.91% <90.90%> (-2.09%) ⬇️
pyomo/contrib/cp/repn/docplex_writer.py 96.47% <95.65%> (-0.09%) ⬇️
...trib/cp/transform/logical_to_disjunctive_walker.py 95.83% <95.83%> (ø)
...rib/cp/transform/logical_to_disjunctive_program.py 96.55% <96.55%> (ø)
pyomo/common/errors.py 100.00% <100.00%> (ø)
pyomo/contrib/cp/__init__.py 100.00% <100.00%> (ø)
pyomo/contrib/cp/plugins.py 100.00% <100.00%> (ø)
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@emma58 emma58 changed the title [WIP] Deprecating logical_to_linear, adding logical_to_disjunctive Deprecating logical_to_linear, adding logical_to_disjunctive Dec 5, 2022
@emma58 emma58 changed the title Deprecating logical_to_linear, adding logical_to_disjunctive Adding logical_to_disjunctive, moving GDP transformations from logical-to-linear to logical-to-disjunctive Dec 5, 2022
@emma58
Copy link
Contributor Author

emma58 commented Dec 5, 2022

@jsiirola, okay, I un-deprecated logical to linear. I think we should rename it logical_to_cnf, but we can do that in a separate PR,

@blnicho blnicho merged commit 9a5681f into Pyomo:main Dec 6, 2022
@jsiirola jsiirola deleted the logical-to-linear-rewrite branch December 9, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants